Conversation
…nd suggest type initializer as a fix.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new PSScriptAnalyzer rule that warns on New-Object usage and provides auto-fix suggestions, along with documentation, localized strings, and Pester tests.
Changes:
- Added
AvoidUsingNewObjectrule implementation with suggested corrections/fixes. - Added rule documentation and registered it in the rules index.
- Added localization strings and comprehensive Pester tests for detection/suppression/fix behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Registers the new rule in the documentation rules table. |
| docs/Rules/AvoidUsingNewObject.md | Adds rule reference documentation and examples. |
| Tests/Rules/AvoidUsingNewObject.tests.ps1 | Adds Pester coverage for rule behavior, suppression, and -fix. |
| Rules/Strings.resx | Adds localized strings for rule name/description/message/correction. |
| Rules/AvoidUsingNewObject.cs | Implements the rule logic and suggested corrections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (boundResult.ConstantValue != null) | ||
| { | ||
| string valueText = boundResult.Value.Extent.Text; | ||
| if ( | ||
| boundResult.Value is StringConstantExpressionAst stringConstant && | ||
| stringConstant.StringConstantType == StringConstantType.BareWord | ||
| ) | ||
| { | ||
| valueText = '"' + valueText.Replace("\"", string.Empty) + '"'; // Test""123 --> "Test123" | ||
| } | ||
| correction = "[" + typeName + "]" + valueText; | ||
| } |
There was a problem hiding this comment.
I think I got this right, if the -ArgumentList contains a constant value, I might use a shorter (preferred) syntax by simply casting the value expression.
For the given example: New-Object DateTime 0 vs [DateTime]0, both return the same results:
Monday, January 1, 0001 12:00:00 AM
iRon7
changed the title
|
Andy Jordan (@andyleejordan), thanks for trying to push this one through. I am currently on holidays, I will check next week on why it failed the workflow. |
|
Apparently I was behind with my fork which caused all kind of errors. All local tests (PowerShell 5.1 and 7) have a green light now... |
|
Christoph Bergmeister (@bergmeister), can I get your approval (or feedback) on this one? I realy would like to see this rule going in to the next PSScriptAnalyzer release. |
|
iRon7 Thanks for the reminder (always good). I think given it's an optional rule, should be OK to add it, nothing I have against it. Kicked off CI tests now |
There was a problem hiding this comment.
🔵 Needs a closer look
Automated corrections can alter executable PowerShell semantics across varied constructor and conversion forms, requiring final human validation.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Christoph Bergmeister (bergmeister)
left a comment
Christoph Bergmeister (bergmeister)
left a comment
There was a problem hiding this comment.
Happy to take this change. There are minor Copilot comments around also alerting in other scenarios and potential edge cases but what the rule does at the moment is good enough for an optional rule, it doesn't need to be perfect
Closes #2046
PR Summary
Avoid using the
New-Objectcmdlet to create objects as it might perform poorly.Instead, use a type initializer to construct or cast the intended object.
Note
In most cases if there isn't an automatic correction (
-fix) available, the rule won't report any violation either.This is because if there isn't an automatic correction available, it generally means that there isn't a simple type-casting or type constructor that would be more efficient or has a better syntax than the using
New-Objectcmdlet.In other words, if e.g. the common
-Verboseparameter is used, or both the parameters-ArgumentListand-Propertyare used, there won't be a simple type initializer available and the rule won't report any violation for theNew-Objectcmdlet.Nevertheless, there are still some cases where the
New-Objectcmdlet might be replaceable with a type initializer that would be more efficient or has a better syntax, but an automatic correction can't be provided.For example if the
-ArgumentListparameter is used with a dynamic variable, the rule will report a violation, but won't be able to provide a correction (-fix), as it's not possible to determine from the AST alone whether the variable contains a single value that can be used in a type initializer, or if it contains multiple values that would require splatting.This PR replaces PR #2109
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.